Conversation
…ction/DNF types, never/true/false/null types Agent-Logs-Url: https://github.com/voku/Simple-PHP-Code-Parser/sessions/42b017a8-8b52-424e-b8c3-4d1f4de1fc44 Co-authored-by: voku <264695+voku@users.noreply.github.com>
Agent-Logs-Url: https://github.com/voku/Simple-PHP-Code-Parser/sessions/42b017a8-8b52-424e-b8c3-4d1f4de1fc44 Co-authored-by: voku <264695+voku@users.noreply.github.com>
…d implode Agent-Logs-Url: https://github.com/voku/Simple-PHP-Code-Parser/sessions/42b017a8-8b52-424e-b8c3-4d1f4de1fc44 Co-authored-by: voku <264695+voku@users.noreply.github.com>
…tory, cache key v5 Agent-Logs-Url: https://github.com/voku/Simple-PHP-Code-Parser/sessions/9701ba04-2500-4da1-925a-c7b46fceb30b Co-authored-by: voku <264695+voku@users.noreply.github.com>
…verride] detection Agent-Logs-Url: https://github.com/voku/Simple-PHP-Code-Parser/sessions/9701ba04-2500-4da1-925a-c7b46fceb30b Co-authored-by: voku <264695+voku@users.noreply.github.com>
Agent-Logs-Url: https://github.com/voku/Simple-PHP-Code-Parser/sessions/9701ba04-2500-4da1-925a-c7b46fceb30b Co-authored-by: voku <264695+voku@users.noreply.github.com>
…e Travis/StyleCI refs Agent-Logs-Url: https://github.com/voku/Simple-PHP-Code-Parser/sessions/9701ba04-2500-4da1-925a-c7b46fceb30b Co-authored-by: voku <264695+voku@users.noreply.github.com>
…xpectation Agent-Logs-Url: https://github.com/voku/Simple-PHP-Code-Parser/sessions/162e52a4-281c-491c-9416-ce79d44f067d Co-authored-by: voku <264695+voku@users.noreply.github.com>
… version-specific syntax files Agent-Logs-Url: https://github.com/voku/Simple-PHP-Code-Parser/sessions/aae312e9-242b-4745-8a50-1c9a59a2cba3 Co-authored-by: voku <264695+voku@users.noreply.github.com>
…constants Agent-Logs-Url: https://github.com/voku/Simple-PHP-Code-Parser/sessions/a7fb28ea-b459-43b6-9a9f-4142c37c21e4 Co-authored-by: voku <264695+voku@users.noreply.github.com>
… PHP 8.2+ syntax Agent-Logs-Url: https://github.com/voku/Simple-PHP-Code-Parser/sessions/fad975c3-c13c-4f59-b333-5ce7469903e1 Co-authored-by: voku <264695+voku@users.noreply.github.com>
… passes Agent-Logs-Url: https://github.com/voku/Simple-PHP-Code-Parser/sessions/4259b945-b0ad-4c21-8980-0db2b5890ade Co-authored-by: voku <264695+voku@users.noreply.github.com>
Agent-Logs-Url: https://github.com/voku/Simple-PHP-Code-Parser/sessions/4259b945-b0ad-4c21-8980-0db2b5890ade Co-authored-by: voku <264695+voku@users.noreply.github.com>
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the library to support modern PHP features (8.0 through 8.3), including attributes, enums, intersection types, DNF types, and typed class constants. It introduces a new PHPAttribute model and a PHPEnum model, while enhancing existing models to capture attribute data and specific PHP 8.3 flags like #[!Override]. Key architectural changes include a two-pass AST traversal to ensure name resolution before model extraction and logic to prevent fatal errors when parsing PHP 8.2+ syntax on older PHP versions. Feedback highlights a critical compatibility issue with nikic/php-parser v4, where a v5-only method was used, and identifies logic errors in type string formatting where backslashes are incorrectly prepended to complex types (Union/Intersection/Nullable). Additionally, a redundant check for the Override attribute name was noted.
| ] | ||
| ) | ||
| ); | ||
| $parser = (new ParserFactory())->createForNewestSupportedVersion(); |
There was a problem hiding this comment.
The method createForNewestSupportedVersion() was introduced in nikic/php-parser v5.0. However, the composer.json in this project specifies "nikic/php-parser": "~4.18". Using this method with v4.x will result in a fatal error. You should use the v4 compatible create() method with ParserFactory::PREFER_PHP7 to support modern PHP syntax.
$parser = (new ParserFactory())->create(ParserFactory::PREFER_PHP7);| public static function typeNodeToString($typeNode): ?string | ||
| { | ||
| if ($typeNode === null) { | ||
| return null; | ||
| } | ||
|
|
||
| if ($typeNode instanceof \PhpParser\Node\NullableType) { | ||
| $inner = self::typeNodeToString($typeNode->type); | ||
|
|
||
| return $inner !== null ? 'null|' . $inner : 'null'; | ||
| } | ||
|
|
||
| if ($typeNode instanceof \PhpParser\Node\UnionType) { | ||
| $parts = []; | ||
| foreach ($typeNode->types as $inner) { | ||
| if ($inner instanceof \PhpParser\Node\IntersectionType) { | ||
| $subParts = []; | ||
| foreach ($inner->types as $subType) { | ||
| $subParts[] = self::typeNodeToString($subType) ?? 'mixed'; | ||
| } | ||
| $parts[] = '(' . \implode('&', $subParts) . ')'; | ||
| } else { | ||
| $parts[] = self::typeNodeToString($inner) ?? 'mixed'; | ||
| } | ||
| } | ||
|
|
||
| return \implode('|', $parts); | ||
| } | ||
|
|
||
| if ($typeNode instanceof \PhpParser\Node\IntersectionType) { | ||
| $parts = []; | ||
| foreach ($typeNode->types as $inner) { | ||
| $parts[] = self::typeNodeToString($inner) ?? 'mixed'; | ||
| } | ||
|
|
||
| return \implode('&', $parts); | ||
| } | ||
|
|
||
| if (\method_exists($typeNode, 'toString')) { | ||
| return $typeNode->toString(); | ||
| } | ||
|
|
||
| if (\property_exists($typeNode, 'name') && $typeNode->name) { | ||
| return $typeNode->name; | ||
| } | ||
|
|
||
| return null; | ||
| } |
There was a problem hiding this comment.
The current implementation of typeNodeToString returns the string representation of class names without a leading backslash. This causes issues in PHPProperty and PHPParameter where a backslash is naively prepended to the entire type string, resulting in invalid types like \null|MyClass or \A|B for union types.
It is better to handle the leading backslash for class names (represented as Name nodes) directly within this helper to ensure consistency across all type formats (nullable, union, intersection, DNF).
public static function typeNodeToString($typeNode): ?string
{
if ($typeNode === null) {
return null;
}
if ($typeNode instanceof \PhpParser\Node\NullableType) {
$inner = self::typeNodeToString($typeNode->type);
return $inner !== null ? 'null|' . $inner : 'null';
}
if ($typeNode instanceof \PhpParser\Node\UnionType) {
$parts = [];
foreach ($typeNode->types as $inner) {
if ($inner instanceof \PhpParser\Node\IntersectionType) {
$subParts = [];
foreach ($inner->types as $subType) {
$subParts[] = self::typeNodeToString($subType) ?? 'mixed';
}
$parts[] = '(' . \implode('&', $subParts) . ')';
} else {
$parts[] = self::typeNodeToString($inner) ?? 'mixed';
}
}
return \implode('|', $parts);
}
if ($typeNode instanceof \PhpParser\Node\IntersectionType) {
$parts = [];
foreach ($typeNode->types as $inner) {
$parts[] = self::typeNodeToString($inner) ?? 'mixed';
}
return \implode('&', $parts);
}
if ($typeNode instanceof \PhpParser\Node\Name) {
return '\\' . $typeNode->toString();
}
if (\method_exists($typeNode, 'toString')) {
return $typeNode->toString();
}
if (\property_exists($typeNode, 'name') && $typeNode->name) {
return (string) $typeNode->name;
}
return null;
}| $typeStr = Utils::typeNodeToString($node->type); | ||
| if ($typeStr !== null) { | ||
| if (\method_exists($node->type, 'getParts')) { | ||
| $this->type = '\\' . $typeStr; | ||
| } else { | ||
| $this->type = $typeStr; | ||
| } | ||
| } elseif (\property_exists($node->type, 'name') && $node->type->name) { | ||
| $this->type = $node->type->name; | ||
| } | ||
| } |
There was a problem hiding this comment.
Prepending a backslash to the result of typeNodeToString is incorrect for complex types. For example, a nullable type ?MyClass becomes null|MyClass, and prepending a backslash results in \null|MyClass. Similarly, union types like A|B become \A|B.
If Utils::typeNodeToString is updated to handle leading backslashes for Name nodes, this logic can be simplified and made correct for all PHP 8.x type variations.
$typeStr = Utils::typeNodeToString($node->type);
if ($typeStr !== null) {
$this->type = $typeStr;
}| $typeStr = Utils::typeNodeToString($parameter->type); | ||
| if ($typeStr !== null) { | ||
| if (\method_exists($parameter->type, 'getParts')) { | ||
| $this->type = '\\' . $typeStr; | ||
| } else { | ||
| $this->type = $typeStr; | ||
| } | ||
| } elseif (\property_exists($parameter->type, 'name')) { | ||
| $this->type = $parameter->type->name; | ||
| } | ||
| } |
There was a problem hiding this comment.
Similar to PHPProperty, prepending a backslash here leads to invalid type strings for complex types (Union, Intersection, Nullable). The formatting should be handled within Utils::typeNodeToString to ensure each class part of a complex type is correctly prefixed.
$typeStr = Utils::typeNodeToString($parameter->type);
if ($typeStr !== null) {
$this->type = $typeStr;
}| // Detect #[\Override] (PHP 8.3+) | ||
| if ($this->is_override === null) { | ||
| foreach ($this->attributes as $attr) { | ||
| if ($attr->name === 'Override' || $attr->name === '\\Override') { |
There was a problem hiding this comment.
The check for \\Override is unnecessary. When using NameResolver, the attribute name is resolved to its fully qualified form without a leading backslash in its string representation (e.g., just Override for the built-in attribute). The \\Override string will never match the output of toString() on a FullyQualified name node in php-parser v4.
if ($attr->name === 'Override') {There was a problem hiding this comment.
Pull request overview
This PR modernizes the Simple PHP Code Parser for PHP 8.x syntax by expanding the parsed model (attributes, enums, newer type features), updating parsing/traversal strategy for correct name resolution, and aligning CI/tooling/docs with the new minimum PHP version.
Changes:
- Add support for PHP 8+ attributes across classes/traits/interfaces/functions/methods/properties/parameters/constants.
- Add enum parsing support (including cases, backing type, methods, constants) and extend container/parser wiring accordingly.
- Update tooling/docs/CI: bump minimum PHP to 8.1, update php-parser + PHPUnit constraints, refresh workflow actions, and adjust README.
Reviewed changes
Copilot reviewed 31 out of 31 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
src/voku/SimplePhpParser/Parsers/PhpCodeParser.php |
Switch to newest-supported parser + split traversal into 2 passes (resolve names before ASTVisitor). |
src/voku/SimplePhpParser/Parsers/Visitors/ASTVisitor.php |
Add Enum_ handling + attach constants to enums/traits. |
src/voku/SimplePhpParser/Parsers/Helper/ParserContainer.php |
Add enum storage + getters/setters. |
src/voku/SimplePhpParser/Parsers/Helper/Utils.php |
Add attribute extraction helpers + robust type-node-to-string conversion + normalize never. |
src/voku/SimplePhpParser/Model/* |
Add PHPAttribute + PHPEnum models; add attributes support to existing models; improve type parsing; add typed-const support. |
tests/ParserTest.php |
Expand test coverage for enums, attributes, intersection/DNF/standalone types, typed consts, Override detection, modern syntax; adjust existing expectations. |
tests/Dummy*.php |
Add fixtures for attributes/enums/new type syntax/trait constants/typed constants/Override and modern syntax. |
README.md |
Document new entities (enums/attributes), requirements, supported features, and update links. |
composer.json |
Require PHP >= 8.1; bump php-parser and PHPUnit dev constraint. |
.github/workflows/ci.yml |
Update PHP matrix (8.1–8.4) and modernize actions/cache/output syntax; adjust phpstan step. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // PHP < 8.2 raises an uncatchable E_COMPILE_ERROR for certain PHP 8.2+ syntax | ||
| // (standalone true/false/null types, DNF types, readonly class). Skip autoloading | ||
| // in that case; AST data is still read from the node below. | ||
| $canAutoload = \PHP_VERSION_ID >= 80200 || !self::nodeUsesPHP82PlusSyntax($node); |
There was a problem hiding this comment.
$canAutoload only guards against PHP 8.2+ syntax on PHP < 8.2, but it doesn’t account for PHP 8.3+ syntax that is also fatal on earlier runtimes (e.g., typed class constants). On PHP 8.2, parsing a PHP 8.3 class that contains typed constants can still trigger an uncatchable compile error when class_exists(..., true) autoloads it. Consider extending the detection logic (or generalizing it) to also detect 8.3+ constructs (e.g., Stmt\ClassConst with non-null type) and gate autoloading on PHP_VERSION_ID >= 80300 for that case.
| - name: Checkout code | ||
| uses: actions/checkout@3df4ab11eba7bda6032a0b82a6bb43b11571feac # v4 | ||
| uses: actions/checkout@v4 | ||
|
|
||
| - name: Setup PHP | ||
| uses: shivammathur/setup-php@2.25.5 | ||
| uses: shivammathur/setup-php@v2 | ||
| with: |
There was a problem hiding this comment.
The workflow switched from pinning actions/checkout/setup-php to a specific commit SHA to using floating tags (@v4, @v2). This weakens supply-chain security because the referenced code can change without review. Prefer pinning third-party actions to an immutable commit SHA (and optionally keeping a comment with the corresponding version).
| - name: Run phpstan | ||
| continue-on-error: true | ||
| if: ${{ matrix.php == '7.4' }} | ||
| if: ${{ matrix.php == '8.3' }} | ||
| run: | | ||
| composer require phpstan/phpstan --dev --no-interaction | ||
| php vendor/bin/phpstan analyse |
There was a problem hiding this comment.
The phpstan step installs dependencies via composer require during CI, which mutates composer.json/composer.lock and makes the run non-reproducible (and can interact poorly with caching). It’s more deterministic to declare phpstan in require-dev (or install it with composer require --no-update plus a composer update that respects the lock), then run vendor/bin/phpstan.
| echo $attr->name; // e.g. "MyAttribute" | ||
| echo $attr->arguments; // constructor arguments |
There was a problem hiding this comment.
The README example uses echo $attr->arguments;, but $arguments is an array, so this will print Array and can emit a notice; it doesn’t demonstrate how to inspect arguments. Consider using print_r($attr->arguments, true), var_export(...), or iterating the array to show key/value pairs.
| echo $attr->name; // e.g. "MyAttribute" | |
| echo $attr->arguments; // constructor arguments | |
| echo $attr->name; // e.g. "MyAttribute" | |
| print_r($attr->arguments); // constructor arguments |
| namespace voku\tests; | ||
|
|
||
| /** | ||
| * Test fixture for PHP 8.1 fibers and first-class callable syntax. |
There was a problem hiding this comment.
The file-level docblock mentions “PHP 8.1 fibers”, but this fixture doesn’t include any Fiber usage (it covers first-class callable, match, named arguments, nullsafe). Consider updating the comment to match what’s actually exercised so it stays accurate over time.
| * Test fixture for PHP 8.1 fibers and first-class callable syntax. | |
| * Test fixture for modern PHP syntax: first-class callables, match, | |
| * named arguments, and the nullsafe operator. |
|
@copilot apply changes based on the comments in this thread |
…equire-dev, README/docblock fixes Agent-Logs-Url: https://github.com/voku/Simple-PHP-Code-Parser/sessions/0f1d61ca-2bd8-4dd3-9c09-972748b9ba0e Co-authored-by: voku <264695+voku@users.noreply.github.com>
Applied all changes from the review thread in commit
Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
|
@copilot Code Review src/voku/SimplePhpParser/Parsers/PhpCodeParser.php The method createForNewestSupportedVersion() was introduced in nikic/php-parser v5.0. However, the composer.json in this project specifies "nikic/php-parser": "~4.18". Using this method with v4.x will result in a fatal error. You should use the v4 compatible create() method with ParserFactory::PREFER_PHP7 to support modern PHP syntax. @voku Reply... @gemini-code-assist The current implementation of typeNodeToString returns the string representation of class names without a leading backslash. This causes issues in PHPProperty and PHPParameter where a backslash is naively prepended to the entire type string, resulting in invalid types like \null|MyClass or \A|B for union types. It is better to handle the leading backslash for class names (represented as Name nodes) directly within this helper to ensure consistency across all type formats (nullable, union, intersection, DNF). @voku Reply... Prepending a backslash to the result of typeNodeToString is incorrect for complex types. For example, a nullable type ?MyClass becomes null|MyClass, and prepending a backslash results in \null|MyClass. Similarly, union types like A|B become \A|B. If Utils::typeNodeToString is updated to handle leading backslashes for Name nodes, this logic can be simplified and made correct for all PHP 8.x type variations. @voku Reply... Similar to PHPProperty, prepending a backslash here leads to invalid type strings for complex types (Union, Intersection, Nullable). The formatting should be handled within Utils::typeNodeToString to ensure each class part of a complex type is correctly prefixed. @voku Reply... The check for \Override is unnecessary. When using NameResolver, the attribute name is resolved to its fully qualified form without a leading backslash in its string representation (e.g., just Override for the built-in attribute). The \Override string will never match the output of toString() on a FullyQualified name node in php-parser v4. @voku Reply... Changes: Add support for PHP 8+ attributes across classes/traits/interfaces/functions/methods/properties/parameters/constants. Show a summary per file src/voku/SimplePhpParser/Model/PHPClass.php Copilot uses AI. Check for mistakes. Copilot AI Copilot uses AI. Check for mistakes. Copilot uses AI. Check for mistakes. Suggested change /**
Suggested change
|
…dundant Override check Agent-Logs-Url: https://github.com/voku/Simple-PHP-Code-Parser/sessions/bc21ded8-d61d-4310-b7d9-67e58a0c5776 Co-authored-by: voku <264695+voku@users.noreply.github.com>
Applied all changes from the Gemini review in commit
All 38 tests pass. |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request significantly updates the library to support PHP 8.0 through 8.3 features, including attributes, enums, intersection types, DNF types, and typed class constants. The minimum PHP requirement is bumped to 8.1, and the parsing logic is refactored into a two-pass traversal to ensure names are fully resolved before model extraction. Additionally, logic was added to prevent uncatchable compile errors when parsing newer syntax on older PHP versions. Feedback focuses on improving type safety when handling reflection types and optimizing class existence checks to account for already-loaded classes.
| if (\method_exists($backingType, 'getName')) { | ||
| $this->scalarType = $backingType->getName(); | ||
| } else { | ||
| $this->scalarType = (string) $backingType; | ||
| } |
There was a problem hiding this comment.
While method_exists works, it is more idiomatic and type-safe to check if $backingType is an instance of ReflectionNamedType before calling getName(), as ReflectionEnum::getBackingType() is guaranteed to return either null or a ReflectionNamedType in PHP 8.1+.
if ($backingType instanceof \ReflectionNamedType) {
$this->scalarType = $backingType->getName();
} else {
$this->scalarType = (string) $backingType;
}| $canAutoload = (\PHP_VERSION_ID >= 80200 || !self::nodeUsesPHP82PlusSyntax($node)) | ||
| && (\PHP_VERSION_ID >= 80300 || !self::nodeUsesPHP83PlusSyntax($node)); |
There was a problem hiding this comment.
The logic to skip autoloading for PHP 8.2+ or 8.3+ syntax on older PHP versions is correct to avoid uncatchable E_COMPILE_ERROR. However, if the class is already loaded in memory (e.g., via a manual require or a different autoloader), we should still attempt to use reflection. Consider checking class_exists($this->name, false) first.
$classExists = \class_exists($this->name, false);
if (!$classExists && $canAutoload) {
try {
if (\class_exists($this->name, true)) {
$classExists = true;
}
} catch (\Throwable $e) {
// nothing
}
}…P 8.1 vs 8.2+ Agent-Logs-Url: https://github.com/voku/Simple-PHP-Code-Parser/sessions/19d4abcb-f02f-4e4f-a240-11bd98857d7c Co-authored-by: voku <264695+voku@users.noreply.github.com>
Agent-Logs-Url: https://github.com/voku/Simple-PHP-Code-Parser/sessions/90cdae65-309d-4e79-b575-cde417342fdb Co-authored-by: voku <264695+voku@users.noreply.github.com>
…ole v7) Agent-Logs-Url: https://github.com/voku/Simple-PHP-Code-Parser/sessions/66364c7f-48bb-4c77-990f-4dfc7140d567 Co-authored-by: voku <264695+voku@users.noreply.github.com>
|
.github/workflows/ci.yml.scrutinizer.yml